Skip to content

fix: duplicate ordering state field names in partial aggregates - #25196

Open
cdelmonte-zg wants to merge 3 commits into
apache:mainfrom
cdelmonte-zg:fix/17715-partial-aggregate-state-fields
Open

fix: duplicate ordering state field names in partial aggregates#25196
cdelmonte-zg wants to merge 3 commits into
apache:mainfrom
cdelmonte-zg:fix/17715-partial-aggregate-state-fields

Conversation

@cdelmonte-zg

@cdelmonte-zg cdelmonte-zg commented Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Partial aggregate state schemas can contain duplicate field names when multiple order-sensitive aggregate expressions use the same ordering expression.

For example, first_value(value ORDER BY timestamp) and last_value(value ORDER BY timestamp) both exposed the ordering state field as timestamp@0.

This required DFSchema::try_from to skip check_names() as a workaround. However, the state_fields() contract requires state field names to be unique within the query.

What changes are included in this PR?

  • Namespace ordering state fields with the aggregate name and the ordering-field position.
  • Apply the same naming scheme to the default AggregateUDFImpl::state_fields() implementation and to FirstValue / LastValue.
  • Re-enable DFSchema::check_names() in TryFrom<SchemaRef> for DFSchema.
  • Update the partial aggregate regression test to assert unique state field names.
  • Add a unit test for the default state_fields() implementation, including duplicate ordering field names within a single aggregate.

For example, an ordering state field now has a name such as:

first_value(value)[ordering_0]

rather than the unqualified:

timestamp@0

The ordering position also disambiguates repeated ordering fields within the same aggregate.

What is the testing strategy for this PR?

The change is covered by:

  • test_partial_aggregate_state_fields_have_unique_names
  • test_default_state_fields_namespaces_ordering_fields

The following test suites pass locally:

cargo test -p datafusion-expr
cargo test -p datafusion-functions-aggregate
cargo test -p datafusion-physical-plan
cargo test -p datafusion-common
cargo test -p datafusion --test core_integration
cargo test -p datafusion-ffi --features integration-tests

cargo fmt --all -- --check and git diff --check also pass.

Are there any user-facing changes?

No public API signatures were changed. A new ordering_state_fields utility was added to centralize the naming of ordering state fields. Ordering state field names now use the aggregate namespace and ordering position to guarantee uniqueness, allowing DFSchema::check_names() to be re-enabled.

@github-actions github-actions Bot added logical-expr Logical plan and expressions core Core DataFusion crate common Related to common crate functions Changes to functions implementation labels Sep 11, 2026
@cdelmonte-zg cdelmonte-zg changed the title fix: Fix duplicate ordering state field names in partial aggregates fix: duplicate ordering state field names in partial aggregates Sep 11, 2026
@github-actions github-actions Bot added the ffi Changes to the ffi crate label Sep 11, 2026
@codecov-commenter

codecov-commenter commented Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.41935% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.88%. Comparing base (e4c4fa4) to head (9a2571e).

Files with missing lines Patch % Lines
datafusion/expr/src/udaf.rs 67.44% 13 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #25196      +/-   ##
==========================================
- Coverage   81.89%   81.88%   -0.01%     
==========================================
  Files        1133     1133              
  Lines      424660   424718      +58     
  Branches   424660   424718      +58     
==========================================
+ Hits       347765   347798      +33     
- Misses      56283    56306      +23     
- Partials    20612    20614       +2

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

jayzhan211
jayzhan211 approved these changes Sep 13, 2026
View reviewed changes

@jayzhan211 jayzhan211 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cdelmonte-zg, all suggestions are non-blocking

Maybe we could also add the change to docs/source/library-user-guide/upgrading/56.0.0.md

Comment thread datafusion/functions-aggregate/src/first_last.rs Outdated
.into(),
];
fields.extend(args.ordering_fields.iter().cloned());
fields.extend(args.ordering_fields.iter().enumerate().map(|(idx, field)| {

@jayzhan211 jayzhan211 Sep 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we add helper to reduce duplication

/// Namespaces ordering fields so state field names stay unique per query.
pub fn ordering_state_fields(name: &str, ordering_fields: &[FieldRef]) -> Vec<FieldRef> {
    ordering_fields
        .iter()
        .enumerate()
        .map(|(idx, f)| {
            Arc::new(f.as_ref().clone().with_name(format_state_name(
                name,
                &format!("ordering_{idx}_{}", f.name()),
            )))
        })
        .collect()
}

@jayzhan211 jayzhan211 Sep 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the first idx might be enough 🤔

first_value(value)[ordering_0_timestamp@0] becomes first_value(value)[ordering_0]

@cdelmonte-zg cdelmonte-zg Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the logic into an helper. As regards the naming scheme, I think you are right. I kept the field name mainly to make debugging easier, but now it is not really needed anymore. Good point 👍

@cdelmonte-zg
cdelmonte-zg force-pushed the fix/17715-partial-aggregate-state-fields branch from 13d4a75 to 9a2571e Compare September 13, 2026 15:58
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Sep 13, 2026
@cdelmonte-zg

cdelmonte-zg commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @cdelmonte-zg, all suggestions are non-blocking

Maybe we could also add the change to docs/source/library-user-guide/upgrading/56.0.0.md

Change added

@cdelmonte-zg

cdelmonte-zg commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Hello @jayzhan211 , thank you for the review! I implemented the changes that you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jayzhan211 jayzhan211 jayzhan211 approved these changes

Assignees

No one assigned

Labels

common Related to common crate core Core DataFusion crate documentation Improvements or additions to documentation ffi Changes to the ffi crate functions Changes to functions implementation logical-expr Logical plan and expressions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Partial AggregateMode will generate duplicate field names from state_fields

3 participants

@cdelmonte-zg @codecov-commenter @jayzhan211